Feat: Add native SDK information in the replay option event #4663
Conversation
brustolin
requested review from
philipphofmann,
armcknight and
philprime
as code owners
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3bf3c92 | 1236.94 ms | 1253.00 ms | 16.06 ms |
| 9ae806a | 1243.84 ms | 1256.22 ms | 12.39 ms |
| b4f8dba | 1343.92 ms | 1362.96 ms | 19.04 ms |
| dc8ab43 | 1220.39 ms | 1238.04 ms | 17.65 ms |
| 8919322 | 1262.67 ms | 1269.90 ms | 7.22 ms |
| 7419285 | 1209.53 ms | 1244.72 ms | 35.19 ms |
| e70a2e1 | 1224.44 ms | 1239.12 ms | 14.68 ms |
| 3f366ee | 1242.28 ms | 1260.80 ms | 18.52 ms |
| 3db3e35 | 1248.02 ms | 1258.35 ms | 10.33 ms |
| 973d574 | 1236.20 ms | 1253.79 ms | 17.59 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3bf3c92 | 21.58 KiB | 706.06 KiB | 684.48 KiB |
| 9ae806a | 21.58 KiB | 616.14 KiB | 594.56 KiB |
| b4f8dba | 21.58 KiB | 614.87 KiB | 593.29 KiB |
| dc8ab43 | 21.58 KiB | 697.83 KiB | 676.25 KiB |
| 8919322 | 22.84 KiB | 403.18 KiB | 380.34 KiB |
| 7419285 | 20.76 KiB | 432.99 KiB | 412.22 KiB |
| e70a2e1 | 21.58 KiB | 655.73 KiB | 634.15 KiB |
| 3f366ee | 20.76 KiB | 427.84 KiB | 407.08 KiB |
| 3db3e35 | 21.58 KiB | 419.21 KiB | 397.63 KiB |
| 973d574 | 21.58 KiB | 542.39 KiB | 520.80 KiB |
Previous results on branch: feat/sdk-info-sr-tags
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7cdca55 | 1240.24 ms | 1256.24 ms | 16.00 ms |
| b7ac501 | 1230.59 ms | 1253.66 ms | 23.07 ms |
| 37adb91 | 1240.06 ms | 1261.49 ms | 21.43 ms |
| 32a98c1 | 1227.61 ms | 1252.87 ms | 25.26 ms |
| 369ece1 | 1233.46 ms | 1257.61 ms | 24.15 ms |
| 8e3d9eb | 1225.58 ms | 1258.66 ms | 33.08 ms |
| 62c1f93 | 1236.84 ms | 1256.41 ms | 19.56 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7cdca55 | 22.32 KiB | 761.54 KiB | 739.22 KiB |
| b7ac501 | 22.32 KiB | 761.95 KiB | 739.63 KiB |
| 37adb91 | 22.31 KiB | 761.39 KiB | 739.08 KiB |
| 32a98c1 | 22.32 KiB | 762.11 KiB | 739.79 KiB |
| 369ece1 | 22.31 KiB | 769.03 KiB | 746.71 KiB |
| 8e3d9eb | 22.31 KiB | 768.84 KiB | 746.52 KiB |
| 62c1f93 | 22.31 KiB | 761.40 KiB | 739.08 KiB |
…sentry-cocoa into feat/sdk-info-sr-tags
Sources/Sentry/include/SentrySessionReplayIntegration+Private.h
Outdated
Show resolved
Hide resolved
Sources/Sentry/include/SentrySessionReplayIntegration+Private.h
Outdated
Show resolved
Hide resolved
Sources/Sentry/include/SentrySessionReplayIntegration+Private.h
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4663 +/- ##
=============================================
- Coverage 91.225% 91.205% -0.021%
=============================================
Files 624 624
Lines 72083 72146 +63
Branches 26221 26256 +35
=============================================
+ Hits 65758 65801 +43
- Misses 6229 6247 +18
- Partials 96 98 +2
... and 18 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -248,6 +248,7 @@ class SentrySessionReplay: NSObject { | |||
| private func captureSegment(segment: Int, video: SentryVideoInfo, replayId: SentryId, replayType: SentryReplayType) { | |||
| let replayEvent = SentryReplayEvent(eventId: replayId, replayStartTimestamp: video.start, replayType: replayType, segmentId: segment) | |||
|
|
|||
| replayEvent.sdk = self.replayOptions.sdkInfo | |||
There was a problem hiding this comment.
we should also set the same sdkInfo to the envelope header, which this replay event is a part of. And from what I see we don't set this sdkInfo to the default global one when initialized, which means we'll nullify the sdk context if we're not running on a hybrid SDK, right? @philprime @philipphofmann could you guys do that or should I take care while Dhiogo is out?
There was a problem hiding this comment.
Not sure if I know how to do that yet, would be great if you or @philipphofmann can do it, with me reviewing the changes so I know afterwards.
There was a problem hiding this comment.
I'm confused by this PR; see #4663 (review).
There was a problem hiding this comment.
I will try to align this PR with how we do this in Java:
There was a problem hiding this comment.
@brustolin, I think we should still align with the Java approach. Using sdkInfo as a dict is acceptable, but I think we should get rid of storing the native SDK info in the Meta.
There was a problem hiding this comment.
I think Java is wrong in this case.
The nativeSDKName and nativeSDKVersion in the replay tags should never be overwritten.
@romtsn Am I missing something here?
There was a problem hiding this comment.
it's up to us I guess.. most important is the version and that one will be correct (i.e. the hybrid SDKs keep the native version in there), and the name doesn't really matter, since we all know it's gonna be android or ios sdk depending on what replay you're watching
There was a problem hiding this comment.
This is PR highly confusing to me. The SentryMeta sdkName and SDKVersion can be changed, and only SR events would use the nativeVersionString and nativeSDKName. All other Sentry data uses the SentryMeta sdkName and sdkVersion. @romtsn, do you use a similar approach on Android? If yes, what are the reasons for that?
Replay needs the information of the native SDK on top of the hybrid SDK info. |
📜 Description
Added cocoa name and version in the session replay options event for easy investigation.
💚 How did you test it?
Unit test
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.🔮 Next steps